Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.08%. Comparing base (e34cd36) to head (a7ceb09).

❗ Current head a7ceb09 differs from pull request most recent head 78bffff

Please upload reports for the commit 78bffff to get more accurate results.

@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
- Coverage   35.17%   34.08%   -1.10%     
==========================================
  Files          61       60       -1     
  Lines        4065     4008      -57     
==========================================
- Hits         1430     1366      -64     
- Misses       2413     2420       +7     
  Partials      222      222              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm thinking that instead of the query parameter, what do you think if we send the Last-Modified and ETag headers in the response?

Last-Modified: {formatted modified_at}
ETag: W/{filepath}-{modified_at}

What do you think?

@fmartingr thanks for idea. i prefer this way but after review some question stil exist for me:

1. we send thumbmail with SendFile with this function
   

   shiori/internal/http/response/file.go

   Lines 13 to 42 in e34cd36

   	func SendFile(c *gin.Context, storageDomain model.StorageDomain, path string) {
   	c.Header("Cache-Control", "public, max-age=86400")
   	if !storageDomain.FileExists(path) {
   	c.AbortWithStatus(http.StatusNotFound)
   	return
   	}
   	info, err := storageDomain.Stat(path)
   	if err != nil {
   	c.AbortWithStatus(http.StatusInternalServerError)
   	return
   	}
   	c.Header("ETag", fmt.Sprintf("W/%x-%x", info.ModTime().Unix(), info.Size()))
   	// TODO: Find a better way to send the file to the client from the FS, probably making a
   	// conversion between afero.Fs and http.FileSystem to use c.FileFromFS.
   	fileContent, err := storageDomain.FS().Open(path)
   	if err != nil {
   	c.AbortWithStatus(http.StatusInternalServerError)
   	return
   	}
   	_, err = io.Copy(c.Writer, fileContent)
   	if err != nil {
   	c.AbortWithStatus(http.StatusInternalServerError)
   	return
   	}
   	}

   
   as you see it set ETag based of info.ModTime().Unix() that Issue with image display in Chrome v1.6.0-rc.7 #857 should not happen at first place but it is happen. first we should fix the problem here that i am not sure why still exist

2. beside that why ETag not work right now i can done same thing we done here for archive files (not use SendFile)
   

   shiori/internal/http/routes/bookmark.go

   Lines 110 to 153 in e34cd36

   	func (r *BookmarkRoutes) bookmarkArchiveFileHandler(c *gin.Context) {
   	ctx := context.NewContextFromGin(c)
   	bookmark, err := r.getBookmark(ctx)
   	if err != nil {
   	return
   	}
   	if !r.deps.Domains.Bookmarks.HasArchive(bookmark) {
   	response.NotFound(c)
   	return
   	}
   	resourcePath, _ := c.Params.Get("filepath")
   	resourcePath = strings.TrimPrefix(resourcePath, "/")
   	archive, err := r.deps.Domains.Archiver.GetBookmarkArchive(bookmark)
   	if err != nil {
   	r.logger.WithError(err).Error("error opening archive")
   	response.SendInternalServerError(c)
   	return
   	}
   	defer archive.Close()
   	if !archive.HasResource(resourcePath) {
   	response.NotFound(c)
   	return
   	}
   	content, resourceContentType, err := archive.Read(resourcePath)
   	if err != nil {
   	r.logger.WithError(err).Error("error reading archive file")
   	response.SendInternalServerError(c)
   	return
   	}
   	// Generate weak ETAG
   	shioriUUID := uuid.NewV5(uuid.NamespaceURL, model.ShioriURLNamespace)
   	c.Header("Etag", fmt.Sprintf("W/%s", uuid.NewV5(shioriUUID, fmt.Sprintf("%x-%x-%x", bookmark.ID, resourcePath, len(content)))))
   	c.Header("Cache-Control", "max-age=31536000")
   	c.Header("Content-Encoding", "gzip")
   	c.Data(http.StatusOK, resourceContentType, content)
   	}

   
   but this make duplicate code that i don't like

3. if i want use SendFile i will change that to get one more input as Etag that if it be empty it generate current Etag logic

do you want approach 2 or 3?

Can you refactor SendFile adding a nilable argument with options coming from a struct?

Something along the lines:

type SendFileOptions struct {
  Headers []http.Header
}


func SendFile(c *gin.Context, storageDomain model.StorageDomain, path string, options *SendFileOptions) {
...
}

And if we don't provide Cache-Control/ETag headers in the options, defaults are set in the function.

What do you think?

@fmartingr i work on that to impeliment ETag there too.
but use of etag not fix #857 it just make less prosses in server side.
the problem is browser can send request for url and if it is not change with cacheBust or anything elese than not send request to update this even if you update Etag.
so still we could follow this approach

• each time send a request(with random buster instead of modifiedAt) to server beside etag (to make things faster)
• use first approach

but combine Etag (improve performance no need each time open that file 7fb2721) with previous approach (add modifiedAt to that url c91ce2c) can be somehow useless in this case because if we used modifiedAt than unnecessary request will not send by default

do you have any idea?

If the best course of action here is to provide a queryparameter with the modified time, let's go for that so we fix the bug. Any improvements can be handled later.

@fmartingr i use queryparamet ?modfiedAt + keep option to define custom etag (now for thumbnail return 304 if no need to update thumbnail and use cache thumbnail instead).
can you please review this on your free time?